Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: resets dict builder keys counter and avoid unnecessary pruning #3386

Merged
merged 3 commits into from
Feb 26, 2024
Merged

fix: resets dict builder keys counter and avoid unnecessary pruning #3386

merged 3 commits into from
Feb 26, 2024

Conversation

evenyag
Copy link
Contributor

@evenyag evenyag commented Feb 26, 2024 •
edited
Loading

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This PR fixes some issues of the new memtable

  • Resets the num_keys counter in the KeyDictBuilder
  • Applies all filters to the partition column
  • Skips unnecessary pk decoding and pruning
  • Skips empty shards

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

Refer to a related PR or issue link (optional)

@evenyag
fix: dict builder resets num_keys on finish
6e46843
@evenyag
feat: skip empty shard and builder
7edb97c
@evenyag
feat: avoid pruning if possible
ff929ee 
Implementations:
- Apply all filters on the partition column
- If no filter to prune, skip decoding keys
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Feb 26, 2024
@evenyag evenyag marked this pull request as ready for review February 26, 2024 08:10
@codecov Codecov
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 85.27%. Comparing base (8087822) to head (ff929ee).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3386      +/-   ##
==========================================
- Coverage   85.74%   85.27%   -0.48%     
==========================================
  Files         893      893              
  Lines      147207   147261      +54     
==========================================
- Hits       126222   125570     -652     
- Misses      20985    21691     +706

v0y4g3r
v0y4g3r approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fengjiachun
fengjiachun approved these changes Feb 26, 2024
View reviewed changes
Copy link
Collaborator

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fengjiachun fengjiachun added this pull request to the merge queue Feb 26, 2024
Merged via the queue into GreptimeTeam:main with commit 26011ed Feb 26, 2024
30 of 32 checks passed
@evenyag evenyag mentioned this pull request Feb 27, 2024
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v0y4g3r v0y4g3r v0y4g3r approved these changes

@fengjiachun fengjiachun fengjiachun approved these changes

Assignees
No one assigned
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evenyag @fengjiachun @v0y4g3r